Code Review for MODEXPS-296: MapStruct Integration

Overview

This code review covers the integration of MapStruct library into the mod-data-export-spring project. The changes include adding MapStruct as a dependency, configuring annotation processors, and implementing object mappers using MapStruct annotations. The primary purpose appears to be replacing manual object mapping logic with compile-time generated mapping code for better performance and maintainability.

Files Changed:

• pom.xml - Added MapStruct dependencies and annotation processor configuration
• src/test/java/org/folio/des/validator/ExportConfigValidatorResolverTest.java - Removed unused ConfigurationClient mock

Files Added/Modified (via MapStruct):

• src/main/java/org/folio/des/mapper/BaseExportConfigMapper.java - Base mapper with MapStruct annotations
• src/main/java/org/folio/des/mapper/aqcuisition/ClaimsExportConfigMapper.java - Claims export mapper
• src/main/java/org/folio/des/mapper/aqcuisition/EdifactExportConfigMapper.java - EDIFACT export mapper
• Test files for the mappers

Suggestions

🔧 MapStruct Annotation Processor Order May Cause Build Issues

Priority: 🔥 Critical

File: pom.xml

Details: The annotation processor path configuration in the maven-compiler-plugin defines the order of annotation processors. The current order places Lombok before the MapStruct binding, but for proper integration between Lombok and MapStruct, the lombok-mapstruct-binding should be specified as a path element with both groupId and artifactId, not just as a nested path within a dependency element.

The current configuration has:

<annotationProcessorPaths>
  <dependency>
    <groupId>org.projectlombok</groupId>
    <artifactId>lombok</artifactId>
  </dependency>
  <path>
    <groupId>org.projectlombok</groupId>
    <artifactId>lombok-mapstruct-binding</artifactId>
    <version>${mapstruct-binding.version}</version>
  </path>
  <path>
    <groupId>org.mapstruct</groupId>
    <artifactId>mapstruct-processor</artifactId>
    <version>${mapstruct.version}</version>
  </path>
</annotationProcessorPaths>

Issue: The first element uses <dependency> while others use <path>. This inconsistency may cause annotation processing order issues, especially when Lombok-generated getters/setters need to be available for MapStruct.

Suggested Change:

<annotationProcessorPaths>
  <path>
    <groupId>org.projectlombok</groupId>
    <artifactId>lombok</artifactId>
    <version>${lombok.version}</version>
  </path>
  <path>
    <groupId>org.projectlombok</groupId>
    <artifactId>lombok-mapstruct-binding</artifactId>
    <version>${mapstruct-binding.version}</version>
  </path>
  <path>
    <groupId>org.mapstruct</groupId>
    <artifactId>mapstruct-processor</artifactId>
    <version>${mapstruct.version}</version>
  </path>
</annotationProcessorPaths>

This ensures consistent configuration and proper ordering. The lombok version should be explicitly specified (it's managed by Spring Boot parent, but being explicit here is better for the annotation processor path).

⚠️ Missing Lombok Version Property

Priority: ⚠️ High

File: pom.xml

Details: The MapStruct annotation processor configuration references Lombok, but the pom.xml doesn't define a <lombok.version> property. While Spring Boot's parent POM manages the Lombok dependency version, the annotation processor path configuration should have an explicit version reference.

Currently, Lombok is defined as:

<dependency>
  <groupId>org.projectlombok</groupId>
  <artifactId>lombok</artifactId>
  <optional>true</optional>
</dependency>

But in the annotation processor path (as suggested above), it needs a version.

Suggested Change: Add to properties section:

<lombok.version>1.18.30</lombok.version>

And ensure consistency between the dependency management and annotation processor versions. Check which version Spring Boot 3.4.3 uses and align with it.

🔧 BaseExportConfigMapper ObjectMapper Modification Side Effect

Priority: 🔥 Critical

File: src/main/java/org/folio/des/mapper/BaseExportConfigMapper.java

Details: The @PostConstruct init() method modifies the autowired ObjectMapper by creating a copy and setting serialization inclusion. However, this pattern has a subtle issue:

@Autowired
protected ObjectMapper objectMapper;

@PostConstruct
public void init() {
  objectMapper = objectMapper.copy().setSerializationInclusion(JsonInclude.Include.ALWAYS);
}

While creating a copy is good (doesn't affect the global ObjectMapper), the field is marked protected and is shared across all mapper implementations. If multiple mappers extend this base class and each is a Spring bean, they each get their own instance, which is correct. However, the code could be clearer.

Concerns:

1. The objectMapper field name is misleading - it's not the original autowired mapper after init, it's a modified copy
2. The modification happens at runtime, which could affect startup performance if many mappers are instantiated
3. The reason for JsonInclude.Include.ALWAYS is not documented

Suggested Change:

@Autowired
private ObjectMapper globalObjectMapper;

protected ObjectMapper objectMapper;

@PostConstruct
public void init() {
  // Create a copy with custom serialization settings for export config mapping
  // ALWAYS inclusion is required to ensure all fields are serialized, 
  // including null values in export configurations
  this.objectMapper = globalObjectMapper.copy()
    .setSerializationInclusion(JsonInclude.Include.ALWAYS);
}

This makes it clear that we're not modifying the global mapper and why we need this specific configuration.

💭 Validation Logic in Mapper Before Mapping

Priority: 🟡 Medium

File: src/main/java/org/folio/des/mapper/aqcuisition/ClaimsExportConfigMapper.java, EdifactExportConfigMapper.java

Details: Both acquisition mappers perform validation in the @BeforeMapping method:

@BeforeMapping
protected void validateConfig(ExportConfig exportConfig) {
  var errors = new BeanPropertyBindingResult(exportConfig.getExportTypeSpecificParameters(), "specificParameters");
  validator.validate(exportConfig.getExportTypeSpecificParameters(), errors);
}

Concerns:

1. The validation is executed but errors are not checked or thrown. If validation fails, the errors object contains the validation errors, but nothing is done with them.
2. This creates a false sense of validation - the method name suggests validation, but it doesn't actually fail the mapping if validation errors occur
3. Mixing validation concerns with mapping concerns violates Single Responsibility Principle
4. If this validation is crucial, it should throw an exception or return errors; if it's just logging, it should be more explicit

Question: What is the intended behavior when validation fails? Should the mapping proceed or should it throw an exception?

Suggested Change:

@BeforeMapping
protected void validateConfig(ExportConfig exportConfig) {
  var errors = new BeanPropertyBindingResult(
    exportConfig.getExportTypeSpecificParameters(), 
    "specificParameters"
  );
  validator.validate(exportConfig.getExportTypeSpecificParameters(), errors);
  
  if (errors.hasErrors()) {
    throw new ValidationException(
      "Export configuration validation failed: " + errors.getAllErrors()
    );
  }
}

Alternatively, if validation should happen before calling the mapper, consider moving it to the service layer where the mapper is called.

🌱 EdifactExportConfigMapper Side-Effect in Validation

Priority: 🟡 Medium

File: src/main/java/org/folio/des/mapper/aqcuisition/EdifactExportConfigMapper.java

Details: The validateConfig method not only validates but also mutates the input object:

@BeforeMapping
protected void validateConfig(ExportConfig exportConfig) {
  // ... validation ...
  
  Optional.ofNullable(ediOrdersExportConfig.getEdiSchedule())
    .map(EdiSchedule::getScheduleParameters)
    .filter(scheduleParameters -> isScheduledParameterIdNotValid(exportConfig, scheduleParameters))
    .ifPresent(scheduleParameters -> {
      log.warn(INCORRECT_UUID_FOR_SCHEDULED_PARAMETER_MSG, exportConfig.getId());
      scheduleParameters.setId(UUID.fromString(exportConfig.getId()));
    });
}

Concerns:

1. This method is mutating the input DTO before mapping, which is a side effect that may not be expected
2. The mutation fixes an "incorrect UUID" but doesn't fail validation - this could hide data quality issues
3. If the same DTO is used elsewhere, this mutation could cause unexpected behavior
4. The warning message uses positional parameters {} which is fine, but consider using structured logging

Observation: This appears to be defensive programming to handle potentially malformed data. While pragmatic, it would be better to either:

• Fail validation and require the caller to provide correct data
• OR explicitly document that this mapper "normalizes" the data as part of the mapping process

Suggested Change: Consider making this behavior more explicit:

@BeforeMapping
protected void normalizeAndValidateConfig(ExportConfig exportConfig) {
  // First normalize the data
  normalizeScheduleParameterId(exportConfig);
  
  // Then validate
  var errors = new BeanPropertyBindingResult(
    exportConfig.getExportTypeSpecificParameters(), 
    "specificParameters"
  );
  validator.validate(exportConfig.getExportTypeSpecificParameters(), errors);
  
  if (errors.hasErrors()) {
    throw new ValidationException("Validation failed", errors);
  }
}

private void normalizeScheduleParameterId(ExportConfig exportConfig) {
  var ediOrdersExportConfig = exportConfig.getExportTypeSpecificParameters()
    .getVendorEdiOrdersExportConfig();
    
  Optional.ofNullable(ediOrdersExportConfig.getEdiSchedule())
    .map(EdiSchedule::getScheduleParameters)
    .filter(scheduleParameters -> isScheduledParameterIdNotValid(exportConfig, scheduleParameters))
    .ifPresent(scheduleParameters -> {
      log.warn("Schedule parameter ID mismatch detected. Expected: {}, Found: {}. Auto-correcting to match export config ID.", 
        exportConfig.getId(), scheduleParameters.getId());
      scheduleParameters.setId(UUID.fromString(exportConfig.getId()));
    });
}

📝 MapStruct Component Model Configuration

Priority: 🟢 Low

File: pom.xml

Details: The compiler configuration includes:

<compilerArg>
  -Amapstruct.defaultComponentModel=spring
</compilerArg>

This is good - it ensures all MapStruct-generated mapper implementations are Spring beans. However, it's worth noting that individual @Mapper annotations in the code don't explicitly specify componentModel = "spring", relying on this default.

Observation: While this works, explicitly specifying componentModel = "spring" in each @Mapper annotation would make the code more self-documenting and less dependent on build configuration. However, the current approach (using the compiler arg) is also a valid pattern and avoids repetition.

Example (alternative approach):

@Mapper(
  componentModel = "spring",
  imports = {ExportConfigConstants.class, ExportTypeSpecificParameters.class}
)

This is a stylistic choice. The current approach is acceptable but less explicit.

👍 Good: Removal of Unused ConfigurationClient Mock

Priority: 🟢 Low

File: src/test/java/org/folio/des/validator/ExportConfigValidatorResolverTest.java

Details: The removal of the unused ConfigurationClient mock bean from the test class is a positive cleanup:

// Removed:
// @MockitoBean
// private ConfigurationClient client;

This reduces test complexity and removes unnecessary mocking. Good practice for keeping tests clean and focused.

⛏ Typo in Package Name

Priority: 🟢 Low

File: src/main/java/org/folio/des/mapper/aqcuisition/ (directory)

Details: The package name is misspelled as aqcuisition instead of acquisition. While this doesn't affect functionality (Java doesn't care about package name spelling), it's a spelling error that should be corrected for professionalism.

Impact: Low - only affects code organization and readability

Suggested Change: Rename the package from aqcuisition to acquisition. This will require:

1. Renaming the directory
2. Updating package declarations in all files within it
3. Updating all imports that reference classes in this package

Note: This is a breaking change if any external code depends on these classes, but since this appears to be an internal package, it should be safe.

📝 MapStruct Expression Complexity

Priority: 🟡 Medium

File: src/main/java/org/folio/des/mapper/BaseExportConfigMapper.java

Details: The mapper uses Java expressions for complex mapping logic:

@Mapping(target = "configName", expression = "java(getConfigName(dto))")
@Mapping(target = "exportTypeSpecificParameters", 
  expression = "java(objectMapper.convertValue(dto.getExportTypeSpecificParameters(), ExportTypeSpecificParameters.class))")

Observation: While this works, extensive use of expression = "java(...)" somewhat defeats the purpose of using MapStruct, which is to have compile-time safe, generated code. The expressions are essentially string-based code that isn't type-checked at compile time.

Alternative Approach: Consider using @Mapping(target = "...", qualifiedByName = "methodName") with @Named methods, which provides better type safety:

@Mapping(target = "configName", qualifiedByName = "getConfigName")
@Mapping(target = "exportTypeSpecificParameters", qualifiedByName = "convertParameters")
public abstract ExportConfigEntity toEntity(ExportConfig dto);

@Named("getConfigName")
protected String getConfigName(ExportConfig dto) {
  return dto.getType() == null || ExportType.BURSAR_FEES_FINES.equals(dto.getType())
    ? DEFAULT_CONFIG_NAME
    : dto.getType().getValue();
}

@Named("convertParameters")
protected ExportTypeSpecificParameters convertParameters(ExportTypeSpecificParameters params) {
  return objectMapper.convertValue(params, ExportTypeSpecificParameters.class);
}

However, the current approach is still valid and may be preferred for its conciseness. This is a stylistic consideration.

💭 ObjectMapper.convertValue Usage Pattern

Priority: 🟡 Medium

File: src/main/java/org/folio/des/mapper/BaseExportConfigMapper.java

Details: The mapper uses objectMapper.convertValue() to convert between the same types:

expression = "java(objectMapper.convertValue(dto.getExportTypeSpecificParameters(), ExportTypeSpecificParameters.class))"

And:

expression = "java(objectMapper.convertValue(entity.getExportTypeSpecificParameters(), ExportTypeSpecificParameters.class))"

Question: Why is convertValue needed when converting from ExportTypeSpecificParameters to ExportTypeSpecificParameters? This appears to be converting a type to itself.

Possibilities:

1. The entity stores parameters as a generic Object or different type, and this ensures type safety
2. This performs a deep copy
3. This ensures serialization/deserialization to handle inheritance or polymorphism
4. The entity actually stores a Map or JSON, and this converts it to the POJO

Observation: Looking at the entity field type would clarify this. If the entity stores the parameters as a different type (e.g., Object, JsonNode, or Map), this makes sense. Otherwise, it might be unnecessary overhead.

Suggested Investigation: Verify what type the entity actually stores. If it's already ExportTypeSpecificParameters, this conversion may be unnecessary. If it's a different type, consider adding a comment explaining the conversion:

// Convert from stored format (Object/JsonNode) to strongly-typed DTO
expression = "java(objectMapper.convertValue(entity.getExportTypeSpecificParameters(), ExportTypeSpecificParameters.class))"

🔧 Missing Null Safety in getConfigName

Priority: ⚠️ High

File: src/main/java/org/folio/des/mapper/aqcuisition/ClaimsExportConfigMapper.java, EdifactExportConfigMapper.java

Details: Both acquisition mappers override getConfigName() with logic that doesn't handle null cases:

@Override
protected String getConfigName(ExportConfig exportConfig) {
  var ediOrdersExportConfig = exportConfig.getExportTypeSpecificParameters()
    .getVendorEdiOrdersExportConfig();
  return exportConfig.getType().getValue() + "_" 
    + ediOrdersExportConfig.getVendorId().toString() + "_" 
    + exportConfig.getId();
}

Concerns:

1. getExportTypeSpecificParameters() could return null → NullPointerException
2. getVendorEdiOrdersExportConfig() could return null → NullPointerException
3. getVendorId() could return null → NullPointerException
4. getType() could return null → NullPointerException

Given that these methods are called during mapping (where validation has supposedly occurred), there's an assumption that all required fields are present. However, defensive programming suggests adding null checks or at minimum, documentation.

Suggested Change:

@Override
protected String getConfigName(ExportConfig exportConfig) {
  Objects.requireNonNull(exportConfig, "exportConfig cannot be null");
  Objects.requireNonNull(exportConfig.getType(), "export type cannot be null");
  
  var specificParams = Objects.requireNonNull(
    exportConfig.getExportTypeSpecificParameters(),
    "export type specific parameters cannot be null"
  );
  
  var ediOrdersExportConfig = Objects.requireNonNull(
    specificParams.getVendorEdiOrdersExportConfig(),
    "vendor EDI orders export config cannot be null"
  );
  
  var vendorId = Objects.requireNonNull(
    ediOrdersExportConfig.getVendorId(),
    "vendor ID cannot be null"
  );
  
  return String.format("%s_%s_%s", 
    exportConfig.getType().getValue(),
    vendorId,
    exportConfig.getId()
  );
}

Alternatively, if validation guarantees these fields are present, add JavaDoc:

/**
 * Generates config name for EDI orders export.
 * 
 * @param exportConfig the export configuration (must have type, specificParameters, 
 *                     vendorEdiOrdersExportConfig, and vendorId populated)
 * @return formatted config name
 * @throws NullPointerException if any required field is null
 */
@Override
protected String getConfigName(ExportConfig exportConfig) {
  // ... existing code ...
}

⛏ String Concatenation vs String.format

Priority: 🟢 Low

File: src/main/java/org/folio/des/mapper/aqcuisition/ClaimsExportConfigMapper.java, EdifactExportConfigMapper.java

Details: The config name generation uses string concatenation:

return exportConfig.getType().getValue() + "_" + ediOrdersExportConfig.getVendorId().toString() + "_" + exportConfig.getId();

Suggestion: For better readability and maintainability, consider using String.format():

return String.format("%s_%s_%s", 
  exportConfig.getType().getValue(),
  ediOrdersExportConfig.getVendorId(),
  exportConfig.getId()
);

This is more readable and makes the format pattern explicit.

📝 Test Coverage for Mapper Implementations

Priority: 🟡 Medium

File: Test files

Details: Based on the visible code, there's a test for DefaultExportConfigMapper but the review should verify that:

1. ClaimsExportConfigMapper has corresponding tests
2. EdifactExportConfigMapper has corresponding tests
3. Tests cover validation failure scenarios
4. Tests cover the schedule parameter ID normalization logic in EDIFACT mapper
5. Tests cover null handling in overridden getConfigName() methods

Observation: Good test coverage exists for the default mapper. Ensure acquisition mappers have similar coverage, especially for their custom validation and naming logic.

🌱 MapStruct Version Consideration

Priority: 🟢 Low

File: pom.xml

Details: The project uses MapStruct 1.6.3, which is the latest stable version as of early 2024. This is good for having the latest features and bug fixes.

Observation: MapStruct 1.6.x introduced better support for Java records, improved null handling, and better integration with Lombok. The current version choice is appropriate.

Future Consideration: Stay updated with MapStruct releases for security patches and improvements, but be aware that major version upgrades may require code changes.

💭 Abstract Class vs Interface for BaseExportConfigMapper

Priority: 🟢 Low

File: src/main/java/org/folio/des/mapper/BaseExportConfigMapper.java

Details: BaseExportConfigMapper is defined as an abstract class with abstract methods. This is necessary because:

1. It needs state (the objectMapper field)
2. It has lifecycle methods (@PostConstruct)
3. It has protected methods shared by subclasses

Observation: The design is appropriate. An abstract class is the right choice here rather than an interface. Good design decision.

⚠️ Missing MapStruct Unmapped Target Policy

Priority: ⚠️ High

File: src/main/java/org/folio/des/mapper/BaseExportConfigMapper.java

Details: The mappers don't specify an unmappedTargetPolicy. Current mappings have explicit ignore = true for some fields:

@Mapping(target = "createdDate", ignore = true)
@Mapping(target = "createdBy", ignore = true)
@Mapping(target = "updatedDate", ignore = true)
@Mapping(target = "updatedBy", ignore = true)

Concern: If the entity gets new fields in the future, and they're not explicitly mapped or ignored, MapStruct will generate warnings during compilation. These warnings can be easily overlooked.

Suggested Change: Add unmappedTargetPolicy to the @Mapper annotation to make the behavior explicit:

@Mapper(
  implementationName = "DefaultExportConfigMapper",
  unmappedTargetPolicy = ReportingPolicy.ERROR,  // Fail build if unmapped targets exist
  imports = {ExportConfigConstants.class, ExportTypeSpecificParameters.class, 
             ExportTypeSpecificParametersWithLegacyBursar.class}
)

This ensures that any future schema changes require explicit mapping decisions, preventing accidental data loss or incorrect mappings.

Summary

The integration of MapStruct into the project is a positive architectural decision that will provide compile-time safe object mapping with better performance than reflection-based approaches. The implementation demonstrates good understanding of MapStruct capabilities, including custom mapping logic, before/after mapping hooks, and Spring integration.

Critical Issues (🔥) - Must Fix:

1. Annotation processor path configuration inconsistency - May cause build failures or incorrect code generation
2. BaseExportConfigMapper ObjectMapper side effects - Unclear code that could lead to maintenance issues
3. Validation logic doesn't throw exceptions - Creates false sense of security

High Priority (⚠️) - Should Fix:

1. Missing Lombok version property - Needed for explicit annotation processor configuration
2. Missing null safety in acquisition mapper getConfigName() - Could cause runtime NPEs
3. Missing unmappedTargetPolicy - Future schema changes could introduce bugs

Medium Priority (🟡) - Consider Fixing:

1. Validation in @BeforeMapping with mutation side effects - Violates SRP and could cause unexpected behavior
2. MapStruct expression complexity - Consider using @nAmed methods for better type safety
3. ObjectMapper.convertValue usage - Verify if conversion is necessary
4. Test coverage for acquisition mappers - Ensure comprehensive testing

Low Priority (🟢) - Nice to Have:

1. Package name typo (aqcuisition → acquisition) - Fix for professionalism
2. String concatenation vs String.format - Better readability
3. Explicit componentModel in @Mapper annotations - Self-documenting code

Positive Observations (👍):

1. Clean removal of unused ConfigurationClient mock
2. Appropriate use of abstract class for base mapper
3. Current MapStruct version (1.6.3) is up-to-date
4. Good test coverage for DefaultExportConfigMapper
5. Proper Spring integration configuration

Recommendations:

1. Address all critical and high-priority issues before merging
2. Add comprehensive tests for acquisition mappers, especially validation scenarios
3. Document the reason for ObjectMapper.convertValue usage if it's intentional
4. Consider adding null safety annotations (@nonnull, @nullable) throughout the mapper classes
5. Review and potentially refactor the validation-in-mapper pattern to separate concerns

The code demonstrates solid engineering practices overall, with the main concerns being around null safety, validation error handling, and some configuration details that could cause issues during builds or maintenance.

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud